New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue-228: Add unorderedSnapshot method to MpscArrayQueue and BaseMps… #229
Conversation
…cLinkedArrayQueue
See comment on main ticket on API. |
@@ -403,7 +417,6 @@ else if (casProducerIndex(pIndex, pIndex + 1)) | |||
{ | |||
final long offset = nextArrayOffset(mask); | |||
final E[] nextBuffer = (E[]) lvElement(buffer, offset); | |||
soElement(buffer, offset, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause GC nepotism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, wasn't aware of this issue. Will try to come up with a solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced this with a marker object that will cause the iterator to jump to the current consumerBuffer.
@@ -73,13 +77,22 @@ final boolean casProducerIndex(long expect, long newValue) | |||
private volatile long consumerIndex; | |||
protected long consumerMask; | |||
protected E[] consumerBuffer; | |||
private volatile E[] volatileConsumerBuffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need 2 consumer buffer refs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I make consumerBuffer volatile and add lp/lv/svConsumerBuffer methods instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does it need to be volatile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware that non-volatile reference writes/reads were always atomic. I guess it doesn't need to be volatile. Thanks.
public List<E> unorderedSnapshot() { | ||
int length = capacity(); | ||
List<E> elements = new ArrayList<E>(); | ||
for (int i = 0; i < length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You iterate through the whole array, very likely to be many thousands of elements, to find the elements you need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to make it less wasteful.
If we're going to add this access we will need some very clearly worded java doc on it's limitations and guarantees. |
…MED to ensure iterator doesn't jump to old buffer
I've updated to provide iterators instead, and believe the GC nepotism issue should be avoided. I made a minor change to the order of writes when replacing the consumer buffer. I would like to write the consumerBuffer field before the BUFFER_CONSUMED marker is inserted, to ensure the iterator doesn't see the BUFFER_CONSUMED flag along with the old consumerBuffer value, which would cause it to scan the same buffer again. Let me know if this isn't okay, and I'll be happy to revert. I couldn't figure out a way to avoid scanning the entire buffer in the linked queues. I'd like to avoid skipping elements that are still queued, and as far as I can tell we will risk doing so if we follow the consumer index. If the iterator sees a mismatch between the consumerIndex and the current buffer, e.g. because the consumer just replaced consumerBuffer, it may start at the wrong position and end up skipping elements. |
Sorry for taking long to re-review, this looks good :-) |
…cLinkedArrayQueue
Resolves #228